Conversation
📝 WalkthroughWalkthroughUpdated a GitHub Actions CI workflow: added Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as GitHub Actions Runner
participant Lint as lint job
participant CheckSkip as check-skip-duplicate job
participant Pulumi as pulumi-workflow job
participant GitHubAPI as GitHub Statuses API
Runner->>Lint: run (new prerequisite)
Runner->>CheckSkip: run
CheckSkip-->>Runner: outputs.should-run
alt event == pull_request and should-run == "true"
Runner->>Pulumi: run (depends on lint)
Pulumi->>GitHubAPI: POST to ${{ github.event.pull_request._links.statuses.href }} (context updated, target_url uses run_id)
GitHubAPI-->>Pulumi: status accepted
else
Runner->>Pulumi: run without posting PR status (duplicate-detection path removed)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@template/.github/workflows/ci.yaml.jinja`:
- Around line 112-120: The status update uses ${ github.event.after } which can
be undefined for pull_request events; replace that with the PR head SHA by using
${{ github.event.pull_request.head.sha }} in the gh api call so the POST to
"/repos/${{ github.repository }}/statuses/${SHA }}" always targets the PR
commit; update the step named "Mark the required-check from the job triggered by
the push as succeeded so the PR can be merged" (the gh api POST invocation that
currently references github.event.after) to use ${{
github.event.pull_request.head.sha }} instead.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@template/.github/workflows/ci.yaml.jinja`:
- Around line 113-114: The step name "Mark the required-check from the job
triggered by the push as succeeded so the PR can be merged" is inconsistent with
its if condition (github.event_name == 'pull_request') and the later
description; update the step name to reference "pull_request" (for example:
"Mark the required-check from the job triggered by the pull_request as succeeded
so the PR can be merged") so the name, condition, and description are
consistent; locate and edit the step name string in the workflow where the if
condition uses needs.check-skip-duplicate.outputs.should-run == 'true'.
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where a required status check was blocking pull request merges. The solution renames the CI job to avoid automatic status creation and explicitly sets the required-check status via API calls only for pull request events.
Changes:
- Renamed the
required-checkjob toworkflow-summaryto decouple the job name from the status check name - Added
lintas a dependency topulumi-workflowto ensure linting runs earlier in the CI sequence - Removed the logic that set status to "pending" when duplicate detection skipped the workflow
- Updated the status-setting step to run only for pull_request events and use the pull request statuses API endpoint
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| -X POST -H "Accept: application/vnd.github.v3+json" \ | ||
| "/repos/${{ github.repository }}/statuses/${{ github.sha }}" \ | ||
| -f state=success -f context="required-check" -f description="✅ All required checks passed" \ | ||
| "${{ github.event.pull_request.statuses_url }}" \ |
There was a problem hiding this comment.
The use of github.event.pull_request.statuses_url is inconsistent with line 119, which uses the explicit path format /repos/${{ github.repository }}/statuses/{sha}. While both approaches should work with gh api, using a consistent format throughout the file would improve maintainability. Consider using "/repos/${{ github.repository }}/statuses/${{ github.event.pull_request.head.sha }}" instead to match the pattern on line 119.
| "${{ github.event.pull_request.statuses_url }}" \ | |
| "/repos/${{ github.repository }}/statuses/${{ github.event.pull_request.head.sha }}" \ |
| "/repos/${{ github.repository }}/statuses/${{ github.sha }}" \ | ||
| -f state=success -f context="required-check" -f description="✅ All required checks passed" \ | ||
| "${{ github.event.pull_request.statuses_url }}" \ | ||
| -f state=success -f context="required-check" -f description="✅ All required checks passed in the job triggered by pull_request" \ |
There was a problem hiding this comment.
The description text "✅ All required checks passed in the job triggered by pull_request" is unnecessarily verbose. The phrase "in the job triggered by pull_request" doesn't add meaningful information. Consider simplifying to "✅ All required checks passed" to match the clarity of the description on line 120.
| -f state=success -f context="required-check" -f description="✅ All required checks passed in the job triggered by pull_request" \ | |
| -f state=success -f context="required-check" -f description="✅ All required checks passed" \ |
| - name: Mark required-check as succeeded | ||
| if: needs.check-skip-duplicate.outputs.should-run == 'true' | ||
| - name: Mark the required-check as succeeded so the PR can be merged | ||
| if: ${{ github.event_name == 'pull_request' }} |
There was a problem hiding this comment.
The condition syntax if: ${{ github.event_name == 'pull_request' }} is inconsistent with the rest of the file, which uses the simpler if: syntax without wrapping the expression in ${{ }} (see lines 44, 59, 86, 113). For consistency, this should be written as if: github.event_name == 'pull_request'.
2 participants
Why is this change necessary?
The previous approach didn't allow merges, because the manually created "processing" status still blocked the merge even after the attempted explicit "success" API call
How does this change address the issue?
change the name of the CI job so it doesn't match the needed status, and have the pull_request job explicitly set the
required-checkstatus to "success"What side effects does this change have?
N/A
How is this change tested?
ejfine/aws-organization#30
pushing commits with an open PR. opening a PR after a successful commit run had finished. closing and reopening PR
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.